Skip to content

KMP migration - #5067

Open
MilosKozak wants to merge 153 commits into
devfrom
kmp
Open

KMP migration#5067
MilosKozak wants to merge 153 commits into
devfrom
kmp

Conversation

@MilosKozak

Copy link
Copy Markdown
Contributor

No description provided.

The blocker for :core:objects was never org.json, it was Dagger. Dagger emits
Java, and the AGP multiplatform library target has NO Java compilation step -
:core:objects:tasks --all lists no *JavaWithJavac task at all, and
build/classes/ holds only kotlin/. So KSP generated CoreModule's
hilt_aggregated_deps and CoreModule_SmsManagerFactory correctly, they were never
compiled, and the app failed with MissingBinding for SmsManager.

That is not fixable with a Gradle setting. The fix is to stop generating.

Five classes - CryptoUtil, RunningModeGuard, QuickWizard, QuickWizardEntry and
BolusWizard - lose @Inject/@singleton and become plain constructors. A new
CoreObjectsModule in :implementation constructs them with @provides, which keeps
the graph identical: same scopes, same instances. This is the trade already made
for BolusProgressData, and its comment says why - javax.inject cannot be used
from code meant to reach commonMain.

javax.inject.Provider is JVM only too, so QuickWizard and QuickWizardEntry take
`() -> T` instead. Dagger still supplies them from the Android side, as a lambda
over Provider.get(). Same pattern as Command.pumpEnactResultProvider.

CoreModule itself moved rather than converted: a @module inside a multiplatform
module would be generated and silently dropped. Its two @ContributesAndroidInjector
entries were dead - nothing member-injects BolusWizard or QuickWizardEntry - so
only the SmsManager @provides came across.

:core:objects now runs no annotation processor: the ksp plugin and all three
Dagger processors are gone from its build file, along with kotlin-parcelize, which
was applied to a module containing no @parcelize at all.

This does not flip the module. org.json still holds ~10 files and has to be dealt
with separately. What it removes is the reason the flip could not work.
:app is the only module that can never become multiplatform - it is the Android
application itself. Wiring put there never has to move again, whereas
:implementation carries no such guarantee.

It also lands among its peers: app/aaps/di/ already holds AppModule,
PluginsListModule and ReceiversModule, and :app already depends on :core:objects.
Hilt aggregates @Installin(SingletonComponent) modules in :app regardless of which
module declares them, so this changes where the source lives and nothing about the
graph.

Worth recording the counter-argument, because it may matter later: :implementation
is probably safe too. It is the ANDROID implementation of :core:interfaces - 42 of
its 116 files import android/androidx directly, including 20 of the 51 *Impl
classes - so iOS would get its own implementation module beside it rather than
sharing this one. But that is a judgement about the future and :app needs no
judgement.

Verified on a cold-booted emulator (-no-snapshot-load): clean start, no
MissingBinding, and the bolus wizard calculates 20 g -> 3.10 U at 10.5 mmol/L,
which exercises BolusWizard through the hand-written provider including the
correction path. No ANR on a quiet machine - the onStartJob ANRs seen earlier were
the build host at load 6.68, not the app.
:core:objects is now a Kotlin Multiplatform module. 17 of 29 files are in
commonMain. The DI lift to :app in the previous commit removed the Dagger
codegen that blocked the flip, and two more files moved once their org.json
boundary was split out:

- BlockExtension and InsulinExtension keep only the kotlinx logic and move to
  commonMain. Their org.json entry points go to new androidMain files
  BlockJsonAdapters and InsulinJsonAdapters. These were already written as thin
  adapters, so nothing else changed.
- ProfileSealed and RunningModeGuard ask for strings by InterfacesStrings
  instead of R.string, and RunningModeGuard takes the common TextResolver
  instead of ResourceHelper.
- System.currentTimeMillis becomes Clock.System, and javaClass.simpleName
  becomes ::class.simpleName.

Still Android only: CryptoUtil (javax.crypto), LoggingWorker (WorkManager), the
two json adapter files, and 8 files that need org.json.

Two Compose deprecations are also cleared:

- compose.components.uiToolingPreview is replaced by
  org.jetbrains.compose.ui:ui-tooling-preview. The new artifact declares the
  annotation under the androidx package name, so all 177 preview files share one
  import with the Android only modules.
- androidx.compose.ui.backhandler.BackHandler is replaced by
  NavigationBackHandler. Note the coordinate: androidx.navigationevent has no
  iOS targets, so this uses the JetBrains republish
  org.jetbrains.androidx.navigationevent:navigationevent-compose.

Two test stubs asked for strings by id while the code moved to TextRef.
SmsCommunicatorPluginTest was a real catch: the guard stopped rejecting and a
bolus went through where the test expected "Pump suspended".

Verified on emulator: profile viewer shows "7.7 g/U" and "8.2 mmol/L/U", the
constraint log keeps its "[Safety]" source tag, and 17 captured screens contain
no unresolved string keys.
The kotlinx halves of our JSON helpers were not equal to the org.json halves
they sit next to. org.json coerces on read and kotlinx throws, so a stored "36"
(a quoted number, which real Nightscout documents contain) read back as 36
through org.json and as the DEFAULT through kotlinx, with nothing logged.

New JsonLenientRead in :core:utils commonMain copies Android's JSON.toInteger /
toLong / toDouble / toBoolean / toString rules, and both sides now share it.

Fixed:

- ICfg.fromJsonObject read insulinEndTime with longOrNull, which answers null for
  1.8E7 and 18000000.5 and fell back to 0. insulinEndTime 0 means DIA 0, so IOB
  decays at once and the loop believes there is no insulin on board. It is
  reachable from InsulinImpl and from BatchActionMapping (client control sync).
- All twelve kotlinx JsonHelper twins had the same coercion gap. The two string
  overloads also had a dead guard: the JsonNull check was overwritten by the next
  line, and because JsonNull is a JsonPrimitive holding "null" they returned that
  text instead of the default.
- SceneSerializer used map with a throwing getString("id"), so ONE damaged scene
  reached the outer catch and returned an empty list, wiping the whole scene
  catalogue. Now the bad entry is skipped, like an unknown action type already
  was.
- QuickWizard.addOrUpdate passed a stale position to JSONArray.put(index, value),
  which pads the list with nulls and makes the readers throw at the next app
  start. remove() had no bounds check and still saved, pushing an unchanged list
  through the sync channel.
- IobTotal.json and determineBasalJson shared one swallowing try, so a NaN iob
  skipped every later put and returned {}, losing time as well.
- QuickWizardEntry left its lateinit storage unassigned on a parse failure, so it
  failed later as UninitializedPropertyAccessException far from the cause.

Removed as dead code:

- The IobTotal.copy() extension. IobTotal is a data class whose fields are all
  constructor parameters, so the generated member copy() wins at every call site
  and nothing imports the extension.
- QuickWizardEntry.usePercentage(), the DEFAULT/CUSTOM constants and the seeded
  "usePercentage": "default" template entry. Its only writer was deleted in
  ccf0fe3 (2023-10-14), so the DEFAULT branch has been unreachable for over
  two years on master as well. IntKey.OverviewBolusPercentage stays - wear still
  uses it.

Split so far: GlucoseValueExtension keeps its logic in commonMain as
toJsonObject, and toJson stays in androidMain as a reparse delegate. Going back
through the text is what keeps the bytes identical, because org.json renders a
whole numbered double as an integer and kotlinx does not.
:core:objects is now 25 files in commonMain and 8 in androidMain. org.json no
longer blocks anything: of the 8 left, 6 are the deliberate boundary adapters
that exist so the ~199 org.json files elsewhere keep compiling, and 2 are real
platform code (CryptoUtil, LoggingWorker).

Four independent files converted, each keeping its logic in commonMain on
kotlinx types and leaving a thin org.json delegate behind:

- IobTotalExtension: plus/round/combine are pure and moved as is; json and
  determineBasalJson became kotlinx builders. The NaN guard is restated for
  kotlinx, which has the opposite failure mode - org.json refuses a non finite
  double, kotlinx accepts it and writes the bare token NaN, which is not valid
  JSON and would turn an uploaded device status into null.
- JSONObjectExt: a clean split, its kotlinx twins already existed. Its commonMain
  store() had the same coercion bug as JsonHelper (strict raw.int / raw.double),
  so it now reads leniently too. Only tests call it today.
- SceneSerializer: its API is String based, so the whole file moved and no
  adapter was needed.
- ProfileSwitchExtension: the units rule is preserved exactly - absent or
  explicitly null falls back to defaultUnits and only a still missing value
  rejects the profile. That rejection matters because GlucoseUnit.fromText never
  throws, it answers MGDL, so a null slipping through would read an mmol/L
  profile as mg/dL and put every target, ISF and correction out by 18x.

QuickWizardEntry, QuickWizard and BolusWizard had to land together because they
depend on each other. The entry now holds plain data instead of a live
JSONObject, and all 23 accessors kept their names so every read call site is
unchanged. Removing the JSONObject removed the aliasing three things relied on:

- setGuidsForOldEntries assigned a guid and never saved. It only persisted
  because the entry WAS the element inside the stored array, so the guid rode
  along on some later unrelated save. If none happened, a different UUID was
  generated on every app start and nothing could resolve a legacy entry by guid
  across restarts. It saves now.
- markAsUsed writes back explicitly through addOrUpdate.
- QuickWizard parses into a list and skips an unreadable element, where a single
  damaged entry used to be a ClassCastException at construction, i.e. the app did
  not start.

The editor's 46 storage.put calls became three typed copy() blocks. Clone now
explicitly keeps its own guid and resets lastUsed instead of relying on which
keys the old code happened to copy.

Last blockers were small: kotlin.concurrent.Volatile, kotlin.uuid.Uuid, and in
BolusWizard the 22 R.string refs to InterfacesStrings plus ResourceHelper to
TextResolver and Calendar to Clock. The bolus calculator is common code now.

Verified on emulator: a created preset persists all 23 keys with the right
defaults (validTo 86340, useBG 0, useCOB 1, percentage 100), clone produces a
second entry with a DIFFERENT guid and lastUsed reset, and both survive a force
stop and restart.
InsulinJsonAdapters is deleted. ICfg.toJson and ICfg.Companion.fromJson had no
references left anywhere but their own declarations - ProfileSwitchExtension was
the last user and moved to fromJsonObject. Checked the neighbours rather than
assuming: determineBasalJson is used by IobTotalTest and IobTotal.json by
LoopPlugin, so IobTotalJsonAdapters stays.

pureProfileFromJson gains a String entry point in commonMain. Four callers held
the profile as text - out of the database or off the Nightscout wire - and built
an org.json document only to hand it straight over. That hop is gone, and two
Nightscout sync files no longer touch org.json at all.

New PureProfileFromJsonParityTest pins the three entry points (String, kotlinx
JsonObject, org.json adapter) against each other, so the remaining callers can be
moved without re-arguing that the profile they read is identical. It also pins
the rules a conversion is most likely to drop quietly:

- a profile without units is REJECTED. This is the one that matters:
  GlucoseUnit.fromText never throws, it answers MGDL, so if a missing unit
  stopped rejecting, an mmol/L profile would be read as mg/dL and every target,
  ISF and correction would be out by 18x.
- values quoted as strings are still read as numbers, which real Nightscout
  documents rely on.
- a missing schedule, and text that is not JSON at all, give an invalid profile
  rather than an exception.
- an unknown timezone falls back to UTC.

The org.json call sites inside core:objects' own tests are left alone on purpose
- they are the only coverage the adapter has.
pureProfileFromJson now has no production caller left on its org.json overload -
only tests use it, and they are the adapter's only coverage.

Build-then-parse removed. DefaultProfile and DefaultProfileDPV assembled a JSON
document only to parse it straight back into a PureProfile, so every value made a
round trip through text. They build the blocks directly now and neither file
contains a single org.json reference. This works because blockFromJson derived
each Block's DURATION from consecutive timeAsSeconds fields, so the helpers that
emitted {time, value, timeAsSeconds} arrays become functions returning
List<Block> with the same arithmetic. Three fields went with the document -
dia, carbs_hr and delay were written but never read back, because none is part of
a PureProfile.

ATProfile.data() and AutotunePlugin.saveLastRun had the same wasted hop:
toPureNsJson already answers a kotlinx document and they rendered it to text just
to reparse it as org.json. Both stay on kotlinx now. AutotunePlugin.loadLastRun
uses the lenient kotlinx readers, so a document written by an older build - where
a whole double was stored as a bare integer - still reads back the same.

ProfileStoreObject is kotlinx throughout. Its own comment said it was waiting for
JsonHelper and pureProfileFromJson to speak kotlinx; both do, so two text round
trips are gone - with() no longer re-serialises the incoming document and
getData() no longer re-parses it on every call.

Found by review of this change:

- ATProfile.data() lost a fail-safe. org.json refused NaN and Infinity outright,
  so a non finite tuned value aborted the document and the method answered null.
  kotlinx writes the bare token NaN and the lenient reader parses it back, which
  would put a NaN into a dosing profile. It now rejects the profile explicitly.
- ProfileStoreObject.getStore() stopped logging a malformed `store`. The old code
  threw out of getJSONObject and logged; answering null silently would hide a
  broken document.
- The getData KDoc still described the old copy-on-read behaviour, and the
  getDefaultProfileName comment stated the old optString rule wrongly - it
  answered the literal text "null" for an explicit null, not "".

Tests: new PureProfileFromJsonParityTest pins the three entry points against each
other, including that a profile without units is REJECTED - GlucoseUnit.fromText
never throws, it answers MGDL, so losing that rejection would read an mmol/L
profile as mg/dL. DefaultProfileTest gains block-boundary assertions, because the
existing midnight-only checks cannot see a duration error, and a case for age 0,
which the old `age > 18` branch never covered.

Verified on emulator: the profile store loads after a force stop and the viewer is
unchanged (24.48 U, Fiasp, IC 7.7 g/U, ISF 8.2 mmol/L/U), and the profile helper
recomputes. Autotune is disabled there with no stored last run, so loadLastRun has
no runtime coverage.
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant